Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix header emitted from gpad writer to match 2.0 specifications #663

Closed
wants to merge 49 commits into from

Conversation

sierra-moxon
Copy link
Member

Note: I did not update the header in the tests that specifically request GPAD 1.2. This PR should not be merged yet.

@sierra-moxon
Copy link
Member Author

@dustine32 - can you take a look at the changes I made to validate.py here? Not ready to merge yet, just wanted a second pair of eyes on the changes I made to carry forward the model details from noctua GPADs through to the final GPADs produced by the pipeline. :)

Copy link
Collaborator

@dustine32 dustine32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems sane to me. Basically separating out the GPAD and TTL product making steps and also mixing GPAD directly from the Noctua GPAD rather than the derived GAF.

bin/validate.py Outdated
))

click.echo("Making noctua gpad products...")
with click.progressbar(iterable=gpadparser.association_generator(file=nf), length=lines) as associations:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only nit-picky thing that you wouldn't really need to change (or I'm reading it wrong) is this lines might be the count of lines in gaf_path above (line 347) and not noctua_gpad_file as I think is intended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing!

@kltm
Copy link
Member

kltm commented Feb 24, 2024

Tag back to geneontology/pipeline#325

@sierra-moxon sierra-moxon marked this pull request as ready for review May 13, 2024 22:15
@sierra-moxon
Copy link
Member Author

#675 should be merged to main, then integrated here (basically a replacement of this PR with that one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants